feat(scanner): add enterprise data protection and database rules - #278
feat(scanner): add enterprise data protection and database rules#278TFT444 wants to merge 7 commits into
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
@ritiksah141 @parthrohit22 - an initial review would be appreciated. |
There was a problem hiding this comment.
Thanks for putting this rule pack together. I went through the implementation and tests carefully. The direction is useful, but I found three things we need to fix before merging:
-
AZ-STOR-009 cannot currently be enabled for a real blob container. The shared policy helper reads
resource.tags, while Azure BlobContainer objects expose metadata rather than ARM tags. That means the documentedoshield:immutability-requiredmarker will never be seen and the rule will silently skip every real container. Please use a supported source of applicability, such as container metadata or an account-level policy, and add a regression test with an SDK-shaped BlobContainer object. -
The NIST mapping for AZ-DB-007 is incorrect.
A.12.4.1is an ISO 27001 control, not a NIST CSF identifier. Please correct it consistently in the rule,nist_csf.json, and the rules reference. If there is no verified direct mapping, use an explicit N/A value. -
Several files presented as remediation playbooks only print guidance. This affects the cache, Cosmos, and SQL rules in this PR. Since the API exposes these files as CLI remediation, either implement safe, validated commands with proper argument handling or classify them clearly as manual guidance instead of executable remediation.
I ran the focused rule tests locally and all 24 passed. The Azure client test module could not collect because this checkout is missing azure.mgmt.authorization, so I did not treat that environment issue as a PR failure. The points above are production contract gaps that the current mocked tests do not cover.
Once these are addressed, I will be happy to take another look.
TFT444
left a comment
TFT444
left a comment
There was a problem hiding this comment.
All three blockers addressed in the latest commit:
- AZ-STOR-009 opt-in check moved from the BlobContainer to the parent storage account, which exposes ARM tags via the SDK. All containers under a tagged account are now evaluated.
- AZ-DB-007 NIST mapping corrected from A.12.4.1 (ISO 27001) to PR.PT-1 across the rule, nist_csf.json, and rules-reference.
- All six echo-only playbooks now contain validated az CLI commands behind an APPLY confirmation gate.
Ready for re-review.
|
@m-khan-97 all three points from your review are addressed. AZ-STOR-009 opt-in now checks both the parent account and the individual container (OR logic, so account-level tagging still covers all containers). AZ-DB-007 NIST mapping corrected to PR.PT-1 across the rule, nist_csf.json, and rules reference. All six echo-only playbooks now contain validated |
|
@TFT444, thanks for addressing the earlier findings. This branch now conflicts with |
@m-khan-97 Conflict have been solved please have a look |
|
@TFT444, I checked the new head directly. The merge conflict is resolved and all 20 checks pass, but one requested cleanup is still missing: Please remove both exclusions and rerun CI. The three substantive rule/playbook fixes are still present after the merge; I will complete their final source review once the security-gate regression is removed. |
|
@m-khan-97 all three items from your review are addressed in the current HEAD: AZ-STOR-009 now checks both the parent account and container for the opt-in tag, AZ-DB-007 NIST mapping is corrected to PR.PT-1, and all six playbooks run validated |
|
@parthrohit22 check the pr please require your approval i believe its good to go |
parthrohit22
left a comment
parthrohit22
left a comment
There was a problem hiding this comment.
Thanks for the work on this rule pack. The scope and opt-in policy model are useful, and the earlier review points appear to be addressed. I found four remaining blockers before I can approve.
-
AZ-STOR-006 skips the insecure Shared Key default.
The rule treats
allow_shared_key_access=Noneas unknown and emits no finding. Azure documentsnullfor this property as equivalent to allowing Shared Key authorization, so default-configured Storage accounts are currently missed.Please treat
Noneas enabled for this rule and add a regression test for that SDK-shaped value. -
AZ-STOR-007 skips the insecure TLS default and does not normalize SDK enums.
Azure documents an unset Storage
minimum_tls_versionas TLS 1.0. The rule currently skipsNone, which suppresses a finding for an insecure default. The SDK property can also be aMinimumTlsVersionenum, but the rule usesstr(value)rather than the existingenum_str()helper.Please normalize the value with
enum_str()and treat an unset value as TLS 1.0. Add tests for bothNoneand an enum-shaped TLS 1.0 value. -
AZ-STOR-008’s CMK playbook parses a standard Key Vault key URI incorrectly.
For a URI such as:
https://<vault>.vault.azure.net/keys/<key-name>/<version>the script passes
https://<vault>.vault.azure.net/keys/<key-name>as the vault URI and<version>as the key name. Azure CLI requires the vault URI, key name, and optional key version separately.Please either accept these as separate parameters or parse the URI correctly and pass
--encryption-key-versionwhen a version is supplied. Add a shell-level regression test or equivalent command-construction test. -
Remove the two reintroduced
pip-auditexclusions.This PR adds exclusions for
CVE-2026-45830andCVE-2026-45833in.github/workflows/ci.yml. These are unrelated to the rule pack and weaken the dependency security gate. Please remove them before merge.
Once these items are addressed and the full CI suite is green on the new head, I will re-review promptly.
Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
|
@m-khan-97 @ritiksah141 @parthrohit22 All the request have been fixed and sort it out i believe it good to go please double check guys . Thank You |
parthrohit22
left a comment
parthrohit22
left a comment
There was a problem hiding this comment.
All 4 items from my last review are fixed on the current head:
- AZ-STOR-006:
Nonenow treated as insecure (matches Azure's documented default), onlyFalseis compliant. - AZ-STOR-007:
Nonetreated as TLS 1.0, and the SDK enum goes throughenum_str()instead of rawstr(). - AZ-STOR-008 playbook: verified the URI-parsing fix directly in bash —
https://vault.../keys/name/versionnow splits into the three separate--encryption-key-vault/--encryption-key-name/--encryption-key-versionargs correctly, with and without a version segment. - The two unrelated pip-audit CVE exclusions are gone.
Ran tests/test_rules_storage.py myself: 22/22 pass, plus bash -n on the playbook.
Approving.
Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
…ules - Move AZ-STOR-009 opt-in check from BlobContainer (no ARM tags) to the parent storage account, which exposes tags via the SDK; all containers under a tagged account are now evaluated for immutability. - Replace incorrect NIST mapping A.12.4.1 (ISO 27001) on AZ-DB-007 with PR.PT-1 across az_db_007.py, nist_csf.json, and rules-reference. - Add executable az CLI commands to fix_az_cache_001, fix_az_cosmos_001, fix_az_cosmos_002, fix_az_db_005, fix_az_db_006, and fix_az_db_007 playbooks; each validates the target and requires APPLY confirmation before modifying any Azure resource. - Update storage-protection-controls.md to document the account-level tagging scope for AZ-STOR-009. Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
…OR-009) The policy_required guard was placed at the account level, but the oshield:immutability-required tag is set per container. Moving the check inside the container loop allows containers with the tag to be evaluated regardless of whether the parent account carries it. Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
…AZ-STOR-009) The policy_required guard was placed at the account level only, but the oshield:immutability-required tag may be set per-container or per-account. Now uses OR logic: a container is evaluated if the account carries the requirement tag (protecting all containers) OR if the container itself carries it (per-container opt-in). Both cases were previously broken: the account-level check did not reach container-tagged resources, and no per-container check existed at all. Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
…7/008 - AZ-STOR-006: treat allow_shared_key_access=None as insecure (Azure documents unset as equivalent to True); only False is compliant - AZ-STOR-007: treat minimum_tls_version=None as TLS 1.0 (Azure default); use enum_str() instead of str() to handle SDK enum objects correctly - AZ-STOR-008 playbook: fix Key Vault URI parsing; the previous bash expansion passed the wrong segments to --encryption-key-vault and --encryption-key-name; now splits vault URI, key name, and optional key version correctly - ci.yml: remove CVE-2026-45830 and CVE-2026-45833 pip-audit exclusions (chromadb CVEs unrelated to this PR; resolved by PR #317) Adds regression tests for None-as-default behavior and SDK enum handling in AZ-STOR-006 and AZ-STOR-007 (22 storage tests, all passing). Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
ca34fb6
a7f180d to
ca34fb6
Compare
5 participants
Summary
Implements issue #261 as a policy-driven Azure data-protection and database-security rule pack.
Added controls
Implementation
Validation
The playbooks are review-safe guidance commands; they do not apply irreversible Azure changes automatically.
Closes #261